[Search][A11Y] Playground -> Open AI form #202071

Samiul-TheSoccerFan · 2024-11-27T20:54:27Z

Summary

refers #195048 (comment)

The goal is to display a high-level error message at the top of the form. We don’t need to show specific details but can provide a general hint, such as There are some errors in the form. When using a screen reader, this message will indicate that the form submission failed due to errors.

Each individual field will then be responsible for displaying its own specific error messages.

This approach ensures compliance with A11Y standards.

PR Screen Record

Add

Add-connector-form.mov

Edit

edit.mov

Checklist

Check the PR satisfies following conditions.

Reviewers should verify this PR satisfies this list as well.

Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support
Unit or functional tests were updated or added to match the most common scenarios

Samiul-TheSoccerFan · 2024-12-02T13:05:51Z

@elasticmachine merge upstream

mdefazio

This should be a danger callout so it's more visible:

<EuiCallout size="s" color="danger" iconType="warning" title="All fields are required" />

Still need the spacer after this.

mdefazio

Thanks for making the update! LGTM

elasticmachine · 2024-12-03T21:01:06Z

Pinging @elastic/response-ops (Team:ResponseOps)

adcoelho · 2024-12-04T09:38:20Z

...tions_ui/public/application/sections/action_connector_form/create_connector_flyout/index.tsx

@@ -159,6 +168,11 @@ const CreateConnectorFlyoutComponent: React.FC<CreateConnectorFlyoutProps> = ({

      const createdConnector = await createConnector(validConnector);
      return createdConnector;
+    } else {


If a form has only required fields (no optional fields)

Having the code here will not meet this requirement. As is the message is triggered whenever the form is not valid.

We should not display a message saying all fields are required when some are optional.

This change will affect all connectors.

Should I restrict this change only to gen AI connectors that we use in playground as most of these have required fields, or should all connectors have the same behavior?

cc: @mdefazio

@adcoelho What if we make the message more generic?

"There are required fields missing" or "Some fields are not valid"

Ideally we will have some message at the top letting the user know there are errors in the form. (which I think is the point of the PR?)

What if we make the message more generic?
"There are required fields missing" or "Some fields are not valid"

These would imo make more sense but I am also trying to be cautious here about a UI change that would affect all connectors. We didn't discuss it internally and weren't aware. But it doesn't seem like a big deal, I'll bring it up in our sync(today).

What if we make the message more generic?
"There are required fields missing" or "Some fields are not valid"

These would imo make more sense but I am also trying to be cautious here about a UI change that would affect all connectors. We didn't discuss it internally and weren't aware. But it doesn't seem like a big deal, I'll bring it up in our sync(today).

Thank you @adcoelho. Please keep us informed about the team's decision.

Ok, getting back to this @mdefazio

What if we make the message more generic?
"There are required fields missing" or "Some fields are not valid"

Won't your suggestion then go against the original comment from @MichaelMarcialis that originated this task and where he explicitly says:

If the form has one or more optional fields, no additional affordance is necessary, beyond marking the optional fields.

?

This change only seems to make sense if it applies to connectors where all fields are required and if not we should leave things as is.

The general idea is that if there are any errors in the form, a header should appear at the top with a message like, There are some errors while submitting. Individual fields or actions can then display their specific error messages for more details. However, I'll let @mdefazio confirm.

Correct—this is about errors in the form. We removed the message "All fields required" that appears initially prior to submission since this wouldn't make sense for all Connector forms.

Related, the original comment is around noting which fields are required, prior to form submission, not about error handling.

If i'm not mistaken, the benefit of this additional callout, is to indicate that there are errors in the form, which may be out of view initially, and provide a landmark and message to screen readers.

If we make this message generic, meaning simply "There are fields with errors.", or "Some fields were not valid", etc., it allows us to show this regardless if all fields are required or not—so for every connector type form.

Someone can correct me on the need and benefit of this, but that is my understanding. I acknowledge that the Connector framework is unique based on the dynamic nature of the forms, so this is a tricky one to start off with for this pattern.

FWIW, my comment wasn't saying that an error callout such a this can't be shown on form submission. I was just stating that a simple note stating that "All items are required" (shown before form submission) isn't to be used on a form that has one or more optional elements. If ya'll feel this form is lengthy, with some elements with validation errors falling out of view, and could benefit from the inclusion of an additional callout indicating errors, I'll leave that to your discretion.

adcoelho · 2024-12-04T09:51:22Z

..._ui/public/application/sections/action_connector_form/create_connector_flyout/index.test.tsx

+    it('show Required all fields message', async () => {
+      const { getByTestId } = appMockRenderer.render(
+        <CreateConnectorFlyout
+          actionTypeRegistry={actionTypeRegistry}
+          onClose={onClose}
+          onConnectorCreated={onConnectorCreated}
+          onTestConnector={onTestConnector}
+        />
+      );
+      await act(() => Promise.resolve());
+
+      await userEvent.click(getByTestId(`${actionTypeModel.id}-card`));
+
+      await waitFor(() => {
+        expect(getByTestId('test-connector-text-field')).toBeInTheDocument();
+      });
+
+      await userEvent.type(getByTestId('test-connector-text-field'), 'My text field', {
+        delay: 100,
+      });
+
+      await userEvent.click(getByTestId('create-connector-flyout-save-btn'));
+
+      expect(onClose).not.toHaveBeenCalled();
+      expect(onConnectorCreated).not.toHaveBeenCalled();
+      expect(getByTestId('required-field-error-label')).toBeInTheDocument();


Suggested change

it('show Required all fields message', async () => {

const { getByTestId } = appMockRenderer.render(

<CreateConnectorFlyout

actionTypeRegistry={actionTypeRegistry}

onClose={onClose}

onConnectorCreated={onConnectorCreated}

onTestConnector={onTestConnector}

/>

);

await act(() => Promise.resolve());

await userEvent.click(getByTestId(`${actionTypeModel.id}-card`));

await waitFor(() => {

expect(getByTestId('test-connector-text-field')).toBeInTheDocument();

});

await userEvent.type(getByTestId('test-connector-text-field'), 'My text field', {

delay: 100,

});

await userEvent.click(getByTestId('create-connector-flyout-save-btn'));

expect(onClose).not.toHaveBeenCalled();

expect(onConnectorCreated).not.toHaveBeenCalled();

expect(getByTestId('required-field-error-label')).toBeInTheDocument();

it('show Required all fields message', async () => {

appMockRenderer.render(

<CreateConnectorFlyout

actionTypeRegistry={actionTypeRegistry}

onClose={onClose}

onConnectorCreated={onConnectorCreated}

onTestConnector={onTestConnector}

/>

);

await userEvent.click(await screen.findByTestId(`${actionTypeModel.id}-card`));

expect(await screen.findByTestId('test-connector-text-field')).toBeInTheDocument();

await userEvent.type(

await screen.findByTestId('test-connector-text-field'),

'My text field',

{

delay: 100,

}

);

await userEvent.click(await screen.findByTestId('create-connector-flyout-save-btn'));

expect(onClose).not.toHaveBeenCalled();

expect(onConnectorCreated).not.toHaveBeenCalled();

expect(await screen.findByTestId('required-field-error-label')).toBeInTheDocument();

The other tests in this file are very outdated(I assume you used those as reference).

screen is preferred over the results of render, findBy* is preferred over getBy*, and await act(() => Promise.resolve()); is not needed(and potentially problematic) when waitFor and findBy should be used to wait for pending promises to be resolved.

Could you please also make sure that in this test all fields are required(and the message shows)? And maybe add another where when not all fields are required the message is not displayed? 🙏

Thank you for the suggestion. I will update these tests accordingly.

adcoelho

I left some comments.

I also noted that the banner does not show up when editing an existing connector and some fields are missing.

Samiul-TheSoccerFan · 2024-12-04T13:37:14Z

I left some comments.

I also noted that the banner does not show up when editing an existing connector and some fields are missing.

Good catch, looks like edit has its own form.

Samiul-TheSoccerFan · 2024-12-04T14:57:32Z

@elasticmachine merge upstream

adcoelho · 2024-12-05T10:57:40Z

...ins/triggers_actions_ui/public/application/sections/action_connector_form/connector_form.tsx

@@ -136,8 +138,19 @@ const ConnectorFormComponent: React.FC<Props> = ({
  useEffect(() => {
    if (onChange) {
      onChange({ isValid: isFormValid, isSubmitted, isSubmitting, submit, preSubmitValidator });
+      if (isFormValid) {
+        resetErrorMessage?.();


isFormValid is available "above" in the flyout(/create_connector_flyout/index.tsx) so it is not necessary to pass this function to the connector form. Why not keep all the logic there instead?

adcoelho · 2024-12-11T10:54:10Z

The banner at the top of the form is shown after submitting the form, but it goes away after onBlur of the last form field. I would expect it to show and disappear on form submit.

The individual form field error messages should disappear on blur though once that field is valid.

This is minor and could be that others feel differently about the expected UX. If this is a minor change, maybe we do it. But open to other opinions of course.

I wouldn't have noticed if you hadn't mentioned tbh.

I don't mind leaving it as is, specially if it is complicated to change.

Samiul-TheSoccerFan · 2024-12-11T18:03:06Z

@mdefazio We are currently depending on the form to decide if it has any errors. If we want only to handle it based on onSubmit, we need to handle this error message using state management. I will do a timebox refactoring to verify. If it turns out to be a big task, I will merge this one as it is.

Samiul-TheSoccerFan · 2024-12-13T17:24:45Z

@mdefazio @adcoelho made some changes and added a new recording, If the code changes or the current UX is not desired, I can revert the commit and bring it back to the original approved state.

hide-message-when-submitted.mov

Samiul-TheSoccerFan · 2024-12-15T16:07:58Z

@elasticmachine merge upstream

elasticmachine · 2024-12-15T18:00:18Z

💛 Build succeeded, but was flaky

Buildkite Build
Commit: 31fc87e

Failed CI Steps

FTR Configs #97

Test Failures

[job] [logs] FTR Configs #97 / Stateful Observability - Deployment-agnostic API integration tests SyntheticsAPITests getSyntheticsMonitors get one monitor should get by id

Metrics [docs]

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id	before	after	diff
`triggersActionsUi`	1.7MB	1.7MB	+1.2KB

History

💚 Build #260447 succeeded d5b647e
💛 Build #258564 was flaky 01110f3
💚 Build #256987 succeeded b2e226c
💛 Build #256276 was flaky c666119
💚 Build #255963 succeeded b3bbe4d

mdefazio · 2024-12-16T14:20:10Z

made some changes and added a new recording, If the code changes or the current UX is not desired, I can revert the commit and bring it back to the original approved state.

++ This makes sense to me. Helps avoid the form jumping as well. Thank you for taking the time to look into this.

kibanamachine · 2024-12-16T15:49:55Z

Starting backport for target branches: 8.16, 8.17, 8.x

https://github.com/elastic/kibana/actions/runs/12356266332

## Summary refers elastic#195048 (comment) The goal is to display a high-level error message at the top of the form. We don’t need to show specific details but can provide a general hint, such as `There are some errors in the form`. When using a screen reader, this message will indicate that the form submission failed due to errors. Each individual field will then be responsible for displaying its own specific error messages. This approach ensures compliance with A11Y standards. ### PR Screen Record #### Add https://github.com/user-attachments/assets/2e6c3304-0ad1-4d84-acc0-f9413e1fd73f #### Edit https://github.com/user-attachments/assets/7f54114c-4b95-40cd-bdec-f506d7151674 ### Checklist Check the PR satisfies following conditions. Reviewers should verify this PR satisfies this list as well. - [x] Any text added follows [EUI's writing guidelines](https://elastic.github.io/eui/#/guidelines/writing), uses sentence case text and includes [i18n support](https://github.com/elastic/kibana/blob/main/packages/kbn-i18n/README.md) - [X] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios --------- Co-authored-by: Elastic Machine <[email protected]> (cherry picked from commit 07c64de)

kibanamachine · 2024-12-16T15:55:37Z

💚 All backports created successfully

Status	Branch	Result
✅	8.16
✅	8.17
✅	8.x

Note: Successful backport PRs will be merged automatically after passing CI.

Questions ?

Please refer to the Backport tool documentation

# Backport This will backport the following commits from `main` to `8.17`: - [[Search][A11Y] Playground -> Open AI form (#202071)](#202071)  ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sqren/backport)  Co-authored-by: Samiul Monir <[email protected]>

# Backport This will backport the following commits from `main` to `8.x`: - [[Search][A11Y] Playground -> Open AI form (#202071)](#202071)  ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sqren/backport)  Co-authored-by: Samiul Monir <[email protected]>

## Summary refers elastic#195048 (comment) The goal is to display a high-level error message at the top of the form. We don’t need to show specific details but can provide a general hint, such as `There are some errors in the form`. When using a screen reader, this message will indicate that the form submission failed due to errors. Each individual field will then be responsible for displaying its own specific error messages. This approach ensures compliance with A11Y standards. ### PR Screen Record #### Add https://github.com/user-attachments/assets/2e6c3304-0ad1-4d84-acc0-f9413e1fd73f #### Edit https://github.com/user-attachments/assets/7f54114c-4b95-40cd-bdec-f506d7151674 ### Checklist Check the PR satisfies following conditions. Reviewers should verify this PR satisfies this list as well. - [x] Any text added follows [EUI's writing guidelines](https://elastic.github.io/eui/#/guidelines/writing), uses sentence case text and includes [i18n support](https://github.com/elastic/kibana/blob/main/packages/kbn-i18n/README.md) - [X] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios --------- Co-authored-by: Elastic Machine <[email protected]>

Samiul-TheSoccerFan added release_note:skip Skip the PR/issue when compiling release notes v9.0.0 Team:Search v8.17.0 a11y Accessibility issue v8.18.0 v8.16.2 labels Nov 27, 2024

Samiul-TheSoccerFan force-pushed the a11y-playground-openai branch from a96b55f to 16ea0af Compare November 29, 2024 00:57

Samiul-TheSoccerFan marked this pull request as ready for review November 29, 2024 00:59

Samiul-TheSoccerFan requested a review from a team as a code owner November 29, 2024 00:59

Samiul-TheSoccerFan added the backport:version Backport to applied version labels label Nov 29, 2024

Samiul-TheSoccerFan requested review from daveyholler and mdefazio November 29, 2024 01:01

mdefazio requested changes Dec 2, 2024

View reviewed changes

Samiul-TheSoccerFan requested review from mdefazio and TattdCodeMonkey December 2, 2024 23:37

mdefazio approved these changes Dec 3, 2024

View reviewed changes

Samiul-TheSoccerFan added the Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) label Dec 3, 2024

adcoelho reviewed Dec 4, 2024

View reviewed changes

adcoelho requested changes Dec 4, 2024

View reviewed changes

adcoelho reviewed Dec 5, 2024

View reviewed changes

Samiul-TheSoccerFan requested review from a team as code owners December 10, 2024 05:06

Samiul-TheSoccerFan added 4 commits December 13, 2024 11:00

Adding header text when all fields are required in the form

382501b

Updating wanring message with callout

3dc0bc1

adding error message in the connector form

bb9f46c

Update to hide error message when submitted in both add and edit mode

d5b647e

Samiul-TheSoccerFan force-pushed the a11y-playground-openai branch from 01110f3 to d5b647e Compare December 13, 2024 17:22

Samiul-TheSoccerFan requested review from mdefazio and removed request for TattdCodeMonkey December 13, 2024 17:26

Merge branch 'main' into a11y-playground-openai

31fc87e

mdefazio approved these changes Dec 16, 2024

View reviewed changes

Samiul-TheSoccerFan added v8.17.1 and removed v8.17.0 labels Dec 16, 2024

Samiul-TheSoccerFan merged commit 07c64de into elastic:main Dec 16, 2024
11 checks passed

kibanamachine mentioned this pull request Dec 16, 2024

[8.16] [Search][A11Y] Playground -> Open AI form (#202071) #204426

Closed

kibanamachine mentioned this pull request Dec 16, 2024

[8.17] [Search][A11Y] Playground -> Open AI form (#202071) #204427

Merged

kibanamachine mentioned this pull request Dec 16, 2024

[8.x] [Search][A11Y] Playground -> Open AI form (#202071) #204428

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Search][A11Y] Playground -> Open AI form #202071

[Search][A11Y] Playground -> Open AI form #202071

Samiul-TheSoccerFan commented Nov 27, 2024 •

edited by kibanamachine

Loading

Samiul-TheSoccerFan commented Dec 2, 2024

mdefazio left a comment

mdefazio left a comment

elasticmachine commented Dec 3, 2024

adcoelho Dec 4, 2024

Samiul-TheSoccerFan Dec 4, 2024

mdefazio Dec 4, 2024

adcoelho Dec 4, 2024

Samiul-TheSoccerFan Dec 4, 2024

adcoelho Dec 5, 2024

Samiul-TheSoccerFan Dec 5, 2024

mdefazio Dec 5, 2024

MichaelMarcialis Dec 9, 2024

adcoelho Dec 4, 2024

adcoelho Dec 4, 2024

Samiul-TheSoccerFan Dec 4, 2024

adcoelho left a comment

Samiul-TheSoccerFan commented Dec 4, 2024

Samiul-TheSoccerFan commented Dec 4, 2024

adcoelho Dec 5, 2024

adcoelho commented Dec 11, 2024

Samiul-TheSoccerFan commented Dec 11, 2024

Samiul-TheSoccerFan commented Dec 13, 2024

Samiul-TheSoccerFan commented Dec 15, 2024

elasticmachine commented Dec 15, 2024

mdefazio commented Dec 16, 2024

kibanamachine commented Dec 16, 2024

kibanamachine commented Dec 16, 2024

[Search][A11Y] Playground -> Open AI form #202071

[Search][A11Y] Playground -> Open AI form #202071

Conversation

Samiul-TheSoccerFan commented Nov 27, 2024 • edited by kibanamachine Loading

Summary

PR Screen Record

Add

Edit

Checklist

Samiul-TheSoccerFan commented Dec 2, 2024

mdefazio left a comment

Choose a reason for hiding this comment

mdefazio left a comment

Choose a reason for hiding this comment

elasticmachine commented Dec 3, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adcoelho left a comment

Choose a reason for hiding this comment

Samiul-TheSoccerFan commented Dec 4, 2024

Samiul-TheSoccerFan commented Dec 4, 2024

Choose a reason for hiding this comment

adcoelho commented Dec 11, 2024

Samiul-TheSoccerFan commented Dec 11, 2024

Samiul-TheSoccerFan commented Dec 13, 2024

Samiul-TheSoccerFan commented Dec 15, 2024

elasticmachine commented Dec 15, 2024

💛 Build succeeded, but was flaky

Failed CI Steps

Test Failures

Metrics [docs]

Async chunks

History

mdefazio commented Dec 16, 2024

kibanamachine commented Dec 16, 2024

kibanamachine commented Dec 16, 2024

💚 All backports created successfully

Questions ?

Samiul-TheSoccerFan commented Nov 27, 2024 •

edited by kibanamachine

Loading